Skip to content

fix code that mixes wavefront index and name#208

Merged
gr5 merged 1 commit intogithubdoe:masterfrom
atsju:JST/fix193
Jul 20, 2025
Merged

fix code that mixes wavefront index and name#208
gr5 merged 1 commit intogithubdoe:masterfrom
atsju:JST/fix193

Conversation

@atsju
Copy link
Collaborator

@atsju atsju commented Jul 20, 2025

This has been introduced in #193

I got complain from newer compiler while migrating to QT6 and I think he is right. One more reason to move.
Actually doThese contains index of wavefront. And it's compared to wavefront name which makes no sense.

Please confirm my (AI powered, human checked) proposal is what you intended to do. It would be great to infer the impact of this to add to changelog bugfix.

@atsju atsju requested review from githubdoe and gr5 July 20, 2025 09:37
@atsju atsju changed the title fix code that uses wavefront index instead name fix code that mixes wavefront index and name Jul 20, 2025
@gr5
Copy link
Collaborator

gr5 commented Jul 20, 2025

There was definitely a bug with that line of code. So you changed the intention of the code a little. here's what it looks like with your new code below. I selected the top 2 waves (9814, 9815) and clicked "subtract wave front" which is the only place this code you edited is used.

As you can see it only gives me the option of subtracting out the remaining wavefront(s) which is just wavefront 9816.

The "old" broken code never subtracted out anything from the gui list. This bug goes back way before #193. For example, the bug exists in version 7.2.3

But the old code appears to try to only subtract out the first selected wave.

I like the way your new code works. I just want to double check with @githubdoe that he likes this new method where it removes all selected wavefronts from the gui list. Do you agree that this is good dale? Should the gui show:

  1. all wavefronts
  2. all wavefronts minus the selected wavefronts
  3. all wavefronts minus the first selected wavefront

I think (2) is what we want and (2) is what Julien did.

Capture

@gr5
Copy link
Collaborator

gr5 commented Jul 20, 2025

I changed my mind on this. Not only do I like this fix, if someone really wants to subtract a wavefront from itself they can just load it twice and give it a different name. Or they can rotate it by 0 degrees (which is one way to make a copy of a wavefront). I'm going to go ahead and merge this fix.

@gr5 gr5 merged commit c7d84b3 into githubdoe:master Jul 20, 2025
8 checks passed
@atsju atsju deleted the JST/fix193 branch July 20, 2025 16:30
@githubdoe
Copy link
Owner

For subtract function I wanted all wave fronts except the selected one but never got around to implementing that.

@atsju atsju mentioned this pull request Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants